-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix all conformance failures other than timeouts/deadlines #274
Conversation
cc @perezd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - just one suggestion.
@@ -36,3 +36,7 @@ internal class EndStreamResponseJSON( | |||
@Json(name = "error") val error: ErrorPayloadJSON?, | |||
@Json(name = "metadata") val metadata: Headers?, | |||
) | |||
|
|||
internal fun contentTypeIsJSON(contentType: String): Boolean { | |||
return contentType == "application/json" || contentType == "application/json; charset=utf-8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use MediaType
to parse these reliably (we use this in ConnectOkHttpClient.kt
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, that is an okhttp
type and helper function. But this is the library
module, which intentionally does not have a dependency on okhttp.
This is what the Go implementation does, so despite being hacky, should be adequate. I'll add a TODO that this could be made more robust in the future.
This PR includes updates to fix all outstanding conformance test failures and removes the corresponding opt-outs. All changes are validated by the conformance test suite. Many of these fixes are similar to connectrpc/connect-kotlin#248 and connectrpc/connect-kotlin#274. Resolves #268. Resolves #269. Resolves #270. This should also be one of the final changes before v1.0 #222. --------- Signed-off-by: Michael Rebello <[email protected]>
The first two commits include fixes to the protocol logic to fix the known failures in conformance tests.
This still leaves the timeout/deadline propagation cases failing since those will require something more significant to resolve.
The third commit is just some minor updates to comments and TODOs that I figured I'd tweak while I was in these files. Hopefully they all make sense.
The final commit cleans up the known failing test case configuration -- to remove everything but the timeout/deadline tests.